clk: qcom: lemans: add QUP SE clock set-rate/DFS/enable support - #26
clk: qcom: lemans: add QUP SE clock set-rate/DFS/enable support#26nnunna94 wants to merge 4 commits into
Conversation
|
Please explain how this was tested and why these changes are needed. |
remove all comments except where you needed - or want to offer- the guidance (LLMs destroy the flow by flooding source code with documentation that is not required). Assume that the user will have access to the same LLM that you do. |
|
currently the validation for this PR APIs is done via unit tests which were excluded in this PR Jorge A. Ramirez-Ortiz (@ldts). End to End validation is planned with buses SPI driver. working with buses POC offline for the same. |
ah, so if nobody is consuming these clocks, can it wait for your next PR? where the clocks are being consumed and the code is used? you could just add the consumer PRs on top of this. I think it would make the patch series better. |
|
Note: |
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clock-qcom-pas.c | ||
| srcs-y += clk-qcom.c | ||
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c | ||
| srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.c |
There was a problem hiding this comment.
CFG_QCOM_CLK_BSP hardcodes clk-qcom-lemans.c instead of clk-qcom-$(PLATFORM_FLAVOR).c
srcs-y += clk-qcom.c
srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c
srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.cThe CFG_QCOM_PAS_PTA line correctly parameterizes on $(PLATFORM_FLAVOR)
for both the source path and the filename. The new CFG_QCOM_CLK_BSP line
one row below parameterizes the directory on $(PLATFORM_FLAVOR) but
hardcodes the filename to clk-qcom-lemans.c. Today this is harmless
because only lemans/target.mk sets CFG_QCOM_CLK_BSP ?= y (confirmed:
kodiak never sets it, and there's no global default anywhere in
mk/config.mk or core/drivers/clk/qcom/sub.mk that would flip it on for
another flavor) — but the moment a second platform enables
CFG_QCOM_CLK_BSP=y, the build will look for
platform/<that-flavor>/clk-qcom-lemans.c, which won't exist, and fail.
Suggest: rename clk-qcom-lemans.c to clk-qcom-$(PLATFORM_FLAVOR).c's
expansion for lemans (i.e. keep today's lemans filename as-is only if that
happens to already match the intended convention) or change the sub.mk
line to platform/$(PLATFORM_FLAVOR)/clk-qcom-$(PLATFORM_FLAVOR).c,
matching the line above it.
There was a problem hiding this comment.
There was a discussion on this file naming on last PR, we discussed on keeping it as clk_bsp.c and you suggest qup.c i beleive. since then i did not rename it... will rename it.
There was a problem hiding this comment.
Yes I remember that discussion. Feel free to rename that if that makes sense.
FYI -> The above comment is slightly different & is not particularly pointing at that. This is related to how we have added the below src include in sub.mk. I want to avoid the explict mention of lemans in the RHS for the file name since that would require you to modify the sub.mk for each platform/cause platform when you try to enable this for some platform other than lemans.
srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-**lemans**.c
For ex: If you enable this for lets say Kodiak, it will search for the file platform/kodiak/clk-qcom-lemans.c instead of clk-qcom-kodiak.c. Hope this particular comment is clear now.
|
Line length: a handful of lines in the frequency-plan tables exceed 80 columns Ensure there are no checkpatch issues in all the commits of this PR & that it can compile for all targets. |
There was a problem hiding this comment.
Pull request overview
This PR adds Qualcomm QUPv3 Serial Engine (QUP SE) clock support for the Lemans (Hoya) platform in OP-TEE, enabling TEE-side SPI/I2C drivers to request, set rates for, and enable DFS on SE clocks without a secure DT.
Changes:
- Introduces a per-target QUP SE clock “BSP” contract (domains + frequency plans + PLL source votes) and registers QUP SE clocks into the common clk framework.
- Adds CMD_DB support for reading auxiliary resource blobs (used to map CX/MX voltage corners to RPMh ordinals for safe set-rate voting).
- Refactors/renames the QCOM clock driver source layout (clock-qcom.c → clk-qcom.c; PAS files similarly) and wires in Lemans BSP build/config.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/include/drivers/qcom/cmd_db/cmd_db.h | Declares cmd_db_get_aux() for retrieving CMD_DB auxiliary blobs. |
| core/drivers/qcom/cmd_db/cmd_db.c | Implements cmd_db_get_aux() and fixes zero-length entry handling. |
| core/include/drivers/clk_qcom.h | Adds QUP SE clock provider APIs (get-by-name, enable DFS, get domain) under CFG_QCOM_CLK_BSP. |
| core/include/drivers/clk_qcom_bsp.h | New BSP data contract types for QUP SE domains, rate plans, and source voting. |
| core/drivers/clk/qcom/clk-qcom.c | New consolidated QCOM clock driver including QUP SE provider, DFS programming, and CX/MX RPMh voting. |
| core/drivers/clk/qcom/clock-qcom.c | Removed legacy QCOM clock driver file (superseded by clk-qcom.c). |
| core/drivers/clk/qcom/sub.mk | Updates build sources to new filenames and adds Lemans BSP compilation under CFG_QCOM_CLK_BSP. |
| core/drivers/clk/qcom/platform/lemans/clock_group_qcom.h | Adds common RCG/DFS register offsets and field masks needed by the walker. |
| core/drivers/clk/qcom/platform/lemans/clk-qcom-lemans.c | Provides Lemans-specific QUP SE domain tables, rate plans, and PLL source vote table. |
| core/drivers/clk/qcom/platform/lemans/clk-qcom-pas.c | Lemans PAS clock enable/reset support under renamed filename. |
| core/drivers/clk/qcom/platform/kodiak/clk-qcom-pas.c | Kodiak PAS support under renamed filename. |
| core/arch/arm/plat-qcom/hoya/lemans/target.mk | Enables CFG_QCOM_CLK_BSP by default and attempts to force CMD_DB/RPMh dependencies. |
Suppressed comments (1)
core/include/drivers/clk_qcom_bsp.h:61
- The qcom_clk_domain documentation describes
@configsas "freq_hz == 0 terminated" and@n_configsas excluding the terminator, but the current clk-qcom implementation uses@n_configsas the iteration bound and does not stop on freq_hz == 0. The doc should match the length-based contract used by the code.
* @configs Frequency-configuration array, freq_hz == 0 terminated.
* @n_configs Number of usable rows in @configs (excluding terminator).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # QUPv3 serial-engine (bus) clock set-rate/DFS walker, consumed on-demand by a | ||
| # future TEE-side SPI/I2C driver. Set-rate votes CX/MX via RPMh, so pull | ||
| # cmd_db/RPMh client in whenever the walker is built. | ||
| ifeq ($(CFG_QCOM_CLK_BSP),y) | ||
| $(call force,CFG_QCOM_CMD_DB,y) | ||
| $(call force,CFG_QCOM_RPMH_CLIENT,y) | ||
| endif | ||
| CFG_QCOM_CLK_BSP ?= y |
| QUP_SE_DOMAIN("gcc_qupv3_wrap1_s3_clk", 0x144f0, qup_se_100mhz, | ||
| GCC_CLOCK_BRANCH_ENA_VOTE_1, 25), |
| /* | ||
| * One frequency configuration row for an RCG. | ||
| * | ||
| * @freq_hz Output frequency in Hz. A terminating row has freq_hz == 0. | ||
| * @mux_sel RCG source-select index (CFG_RCGR SRC_SEL), pre-resolved. | ||
| * @div2x Twice the half-integer source divider; the register encodes | ||
| * SRC_DIV as (div2x - 1), 0 means no divide. |
| /* GCC-relative BSP offsets need a GCC mapping; register one if PAS PTA didn't. */ | ||
| #ifndef CFG_QCOM_PAS_PTA | ||
| register_phys_mem(MEM_AREA_IO_NSEC, GCC_BASE, GCC_SIZE); | ||
| #endif |
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clock-qcom-pas.c | ||
| srcs-y += clk-qcom.c | ||
| srcs-$(CFG_QCOM_PAS_PTA) += platform/$(PLATFORM_FLAVOR)/clk-qcom-pas.c | ||
| srcs-$(CFG_QCOM_CLK_BSP) += platform/$(PLATFORM_FLAVOR)/clk-qcom-lemans.c |
f04d2b3 to
ded401b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
core/drivers/clk/qcom/clk-qcom-vreg.c:82
- All errors from the MX lookup are currently treated as “MX is absent.” In particular, a transient
TEE_ERROR_BAD_STATEor corrupted command DB would permanently setreadyand allow rate changes with CX-only votes. OnlyTEE_ERROR_ITEM_NOT_FOUNDshould make MX optional; propagate other failures so initialization can be retried safely.
/* MX tracks CX on this target; vote it too when the rail exists. */
if (!cmd_db_get_addr("mx.lvl", &v->mx_addr))
v->have_mx = true;
core/drivers/clk/qcom/clk-qcom.c:466
- When the CLK_OFF poll times out, the function returns an error but leaves this SE's shared branch-vote bit asserted. The clock core therefore keeps its enable count at zero while hardware remains requested on, leaking power and making the failed enable non-transactional. Clear the vote bit before returning the timeout.
REG_POLL_TIMEOUT(cbcr, 10 * 1000, 10, &ret, cbcr_branch_on);
return ret < 0 ? TEE_ERROR_TIMEOUT : TEE_SUCCESS;
core/drivers/clk/qcom/clk-qcom.c:482
- Disabling an SE clears only its branch vote; it never removes
qup->cornerfrom the aggregate CX/MX reference counts. Since the first successfulclk_set_rate()adds that reference even while the clock is disabled, every SE ever configured permanently pins the rails at its historical corner. Release the corner on the final disable and reacquire it before the next enable, while retaining the configured corner separately for rate reporting/re-enable.
cbcr = gcc_base + dom->cbcr_offset;
io_clrbits32(gcc_base + dom->vote_reg_offset, BIT(dom->vote_bit));
core/drivers/clk/qcom/clk-qcom.c:420
rateis anunsigned long, but passing it to theuint32_thelper silently truncates requests aboveUINT32_MAX. On AArch64, for example, 4 GHz + 294,967,296 Hz becomes zero and resolves to the plan's minimum rate instead of being rejected. Validate the public clock API value before narrowing it.
struct qcom_qup_clk *qup = clk->priv;
uint32_t res_hz = 0;
TEE_Result res = TEE_SUCCESS;
res = qcom_domain_set_rate(qup->domain, rate, &qup->corner, &res_hz);
| /* Raise the rail before speeding up; abort on failure. */ | ||
| if (next > prev) { | ||
| res = qcom_clk_vreg_vote(prev, next); | ||
| if (res) | ||
| return res; | ||
| *corner = next; | ||
| } |
| clk = clk_alloc(bsp->domains[i].name, &qcom_qup_clk_ops, | ||
| NULL, 0); | ||
| if (!clk) | ||
| return TEE_ERROR_OUT_OF_MEMORY; |
nnunna94, If this is also waiting for consumers/dependent PRs to be ready, Can you please move this as well to draft Naresh? |
#33 i think spi changes are also in review here..once done we can merge together . |
It needs to be raised a single committable PR with the consumer Naresh; So ideally the SPI changes PR can pull these changes with your signed off/author/etc. and have it as part of their PRs. Jorge A. Ramirez-Ortiz (@ldts), Hope that makes sense? |
that should be ok but this becomes a big PR... FYI.. the pr will have clock, tlmm and spi driver all three ..since this is a dependency for spi team, this landed a PR first i believe... |
Just checked the number of lines it'd be <= 4600 in its current state if we combine the three PRs and if we clean up the commits/remove comments etc. it might reduce further as well so should be Ok I think; The fuse PR also was almost around those many lines. |
|
please rework all the abstractions and propose again:. you have a added a new driver (a voltage regulator?) and hidden it under clock. doesnt seem like something we want to do. if it is just a clock helper but unrelated to the actual clock, what about just qcom_corner_vote.c, or qcom_rail_vote.c? |
|
rename -bsp.c to -cfg.c |
Thats just a file to aggregate on voltage requests from clock driver side and send it to RPMH driver. this voting mechanism changes based on architecture so had to separate it out from clk-qcom.c to support both shikra and lemans with same clk-qcom.c the file name may be adjusted accordingly. |
so what is the abstraction then? is this common to architecture or platform? do you need to move clk/qcom/platform to clk/qcom/$arch/$platform and then have clk/qcom/hoya/qcom_corner_vote.c and clk/qcom/qcom_corner_vote.h? |
The idea was to use this file for all hoya and wildcat architectures, for any rpm based architectures introduce a qcom_vreg_rpm.c file or so and based on chipset we decide which file to include in mk files keeping the API interfaces same for clk-qcom.c to remain the same. As of now we did not introduce any architecture folders as such. |
ok so it is not per architecture then. so lets have qcom_vreg_rpmh.c and qcom_vreg.h? the problem with vreg is that one expects a regulator driver...not sure if we can come with a better name equally short.. |
There was a problem hiding this comment.
Remove AI slop from commit messages. But please do it globally or we will never get this merged.
For example, why do you need to add this to the commit message?
While here, make copy_aux_data() return TEE_ERROR_SHORT_BUFFER instead
of silently truncating an undersized read (unreached by either
existing caller, which both query metadata only), and clear
result->len on the zero-length-entry path cmd_db_get_aux() exercises.
Also Remove unnecessary AI comments from the code (those comments were written by AI to explain you the logic it was using: unless you consider them really necessary - something that you wouldn't be able to figure out- you don't need to commit those.
Do not create a new file in a commit and then rename it on the subsequent commit in the same series: just fix it on the first commit.
Do not add me as a co-author: I am just reviewing and providing suggestions.
| return TEE_ERROR_BAD_STATE; | ||
| } | ||
|
|
||
| res = cmd_db_get_entry_by_res_id(res_id, &result, buf); |
There was a problem hiding this comment.
this i/f API is static within cmd_db.c, if it's exported than we don't need additional wrapper?
Selvam Sathappan (@zelvam95) can you check new i/f for rpmh & cmd_db.
There was a problem hiding this comment.
Hi Dinesh,
When I pushed Fuse Mgmt for Hoya, I was supposed to only make those APIs required by Fuse drivers as public. That is the only rationale for current design.
If we don't have any concerns with clients calling them directly, we could definitely make them non-static / public as/when required.
There was a problem hiding this comment.
On that note -> In TF-A also I believe cmd_db_get_aux is a separate public API exposed to clients (In Sumit's series IIRC). So, if it'd make sense to align with TF-A, we can align here.
In general If multiple cmd_db_get_aux clients will be there, then it'd make sense to expose it as separate API like how we've done now along with those additional checks etc. (to avoid duplicate checks in each client/calling place) but if we will have only one or so caller, then we won't need wrapper and we can directly expose the cmd_db_get_entry_by_res_id by making it as non-stack and adding to header (basically we can try to expose the required abstractions to clients and ensure the code overall is concise/clean/doesn't have redundant checks)
ea72266 to
8b937da
Compare
Jorge A. Ramirez-Ortiz (@ldts) / Selvam Sathappan (@zelvam95) i have addressed the rpmh_vote.h file naming issue, the comments generated by AI issue, the commit messages clean ups to most extent. the one thing left to address is the #if def inside .c file changes. the problem i am seeing here with the header file approach is that some of my functions inside the ifdef are static ones not declared in any header files, these are local to the .c file. The header file solution doesnt really solve the problem for here i believe since those are not public or declared in header fiels to add stubs in header files. Also making them non static is also not really gonna solve the problem as well i believe. one solution is to move all this new functions and APIs to a new file outside of clk-qcom.c and make things align to standards. for now, i am implementing this, let me know if you see any concern here.i borrowed the name for that file from linux usptream.. since most of the operations in that file revolve around rcg configuration.... Also, the one thing left out... the cmd_db related commit, i would need to work it out with rpmh team IDineshChoudhary further if i can get a PR for this and the fix identified on the MSG ID length issue or should we refactor it here on the same PR further. |
|
looks good to me from soc infra. |
| # Aggregate CX/MX rail-corner voting behind the set-rate path. RPMh targets | ||
| # build the RPMh backend; without it a CLK_CFG build fails to link rather than | ||
| # silently running under-volted. | ||
| ifeq ($(CFG_QCOM_CLK_CFG),y) |
There was a problem hiding this comment.
why not just add
| ifeq ($(CFG_QCOM_CLK_CFG),y) | |
| ifeq ($(CFG_QCOM_CLK_CFG),y) | |
| srcs-y += platform/$(PLATFORM_FLAVOR)/clk-qcom-cfg.c | |
| srcs-$(CFG_QCOM_RPMH_CLIENT) += rpmh_rail.c | |
| srcs-y += clk-qcom-rcg.c | |
| endif |
8b937da to
c4d7c0d
Compare
| static const struct qcom_clk_mux_config * | ||
| find_config(const struct qcom_clk_domain *domain, uint32_t freq_hz) | ||
| { | ||
| const struct qcom_clk_mux_config *at_least = NULL; |
There was a problem hiding this comment.
at_least is a very unusual choice - never seen this upstream.
There was a problem hiding this comment.
That variable is named accordingly since it is lock a config that is going to satisfy the request and what would be the at least config that would do the job here.
There was a problem hiding this comment.
I think the point is "at least" for a variable name seems kinda odd.
We could use "min_config" or something on those lines? Do we have this driver in Kernel? We could look that up as well for ref?
There was a problem hiding this comment.
yes I think we need to think this re-structuring a bit more and align as much as possible with the work already done in the kernel in terms of patterns, variable names and functions. Just by doing that we will probably come with better abstractions (the right semantics often lead to the proper interfaces)
There was a problem hiding this comment.
my worry is how this is going to evolve. we could start by removing clk-qcom- from the implementation filenames in clk/qcom/* and just use clk-xxx instead. But still keep clk_qcom.h for the users.
| struct qcom_clk_priv { | ||
| const struct qcom_clk_domain *domain; | ||
| struct clk *clk; | ||
| vaddr_t cbcr_va; |
There was a problem hiding this comment.
you can drop the type at the end of the variable name
| vaddr_t cmd_rcgr_va; | ||
| uint16_t corner; /* corner voted for this domain */ | ||
| uint32_t rate; /* last resolved output rate, 0 until set */ | ||
| bool dfs_on; /* hardware DFS already handed the RCG */ |
There was a problem hiding this comment.
s/dfs_on/dfs
| }; | ||
|
|
||
| static const struct qcom_clk_src_vote qup_se_src_votes[] = { | ||
| { &gcc_regmap, MUX_GPLL0_DIV2, GCC_PLL_BRANCH_ENA_VOTE, |
There was a problem hiding this comment.
This will break the functionality if different sources (plls) have same MUX value.
Vinod Kumar Amanaganti (vinod-amanaganti)
left a comment
Vinod Kumar Amanaganti (vinod-amanaganti)
left a comment
There was a problem hiding this comment.
please address the review comments.
|
|
||
| #define QUP_SE_DFS_STATES 8 | ||
|
|
||
| #define CX_MIN_SVS 0x30 |
There was a problem hiding this comment.
do these numbers vary from chip to chip? if not can you move them to a common header?
| .disable = clk_priv_disable, | ||
| .set_rate = clk_priv_set_rate, | ||
| .get_rate = clk_priv_get_rate, | ||
| }; |
There was a problem hiding this comment.
can you add the support for get_parent, set_parent, get_rates_array like other vendors in the tree
| * consumer walks domain->configs[] to run its own divider search and takes the | ||
| * chosen row's DFS index from its dfs_idx field. | ||
| */ | ||
| const struct qcom_clk_domain *qcom_clk_get_domain(struct clk *clk); |
There was a problem hiding this comment.
Can we avoid this by making use of get_rates_array API? for dfs enabled domains, we can provide dfs enabled frequencies
|
|
||
| for (i = 0; i < qcom_clk_priv_count; i++) { | ||
| if (!strcmp(qcom_clk_privs[i].domain->name, name)) { | ||
| *out = qcom_clk_privs[i].clk; |
There was a problem hiding this comment.
Isn't it good to expect the caller to pass the "struct clk" argument and we can fill it instead of returning the clk address defined in the driver?
| static unsigned long clk_priv_get_rate(struct clk *clk, | ||
| unsigned long parent_rate __unused) | ||
| { | ||
| struct qcom_clk_priv *priv = clk->priv; |
There was a problem hiding this comment.
Can we directly measure the frequency from hardware by making use of ring-oscillator instead of relying on variable? As this API can still return non-zero, if the clock scaled and then turned off.
| for (i = 0; i < cfg->n_src_votes; i++) { | ||
| const struct qcom_clk_src_vote *sv = &cfg->src_votes[i]; | ||
|
|
||
| if (sv->mux_sel != mux_sel || sv->regmap != regmap) |
There was a problem hiding this comment.
This logic will pick the first source entry of qup_se_src_votes array instead of picking the right entry when there are multiple sources that maps same mux value. please modify this logic
This file already lives under drivers/clk/qcom/, so repeating qcom in its own name is redundant, and inconsistent with how other vendor subdirectories (sam/, stm32) name their own files. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude Opus 5
The QUP SE clock driver's CX/MX voltage vote needs a rail's supported corner ordinals, which for ARC resources live in the auxiliary data blob RPMh commands index into rather than a raw voltage. Add cmd_db_get_aux() to fetch it by resource ID. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude Opus 5
c4d7c0d to
f33870e
Compare
Some clock rates need a higher CX/MX voltage corner than others, and a corner may be shared by multiple RCGs; rail_vote() refcounts per corner over RPMh so each rail is only raised for as long as some caller actually needs it, and CX/MX are resolved independently since they don't necessarily share the same hlvl encoding. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude Opus 5 Assisted-by: Claude Sonnet 5
f33870e to
b925381
Compare
| return TEE_SUCCESS; | ||
| } | ||
|
|
||
| static const struct clk_ops rcg_ops = { |
There was a problem hiding this comment.
Rail vote is never released when the clock is disabled.
rcg_ops has no .disable hook, and the CX/MX rail vote is only lowered inside rcg_set_rate() when a lower rate is subsequently requested (next_corner < curr_corner). So once an RCG is set to a high rate, priv->corner keeps the rail asserted even after the branch is gated off — and it stays up until someone calls set_rate with a lower corner, which a disabled consumer typically will not do.
For the on-demand SPI/I2C consumer this targets (enable → transact → disable), the SVS-corner vote on CX/MX leaks for the lifetime of the system after the first transaction.
Is this intentional (rate persists in hardware across gating, so the rail must stay up)? If so, please add a comment documenting the choice. Otherwise the rail vote should be dropped on disable and re-asserted on enable so each rail is only held while some consumer actually needs it — which also matches the per-corner refcounting rationale in the rail_vote commit.
There was a problem hiding this comment.
with the above addressed
Reviewed-by: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>
There was a problem hiding this comment.
addressed this with proper disable ops.
| if (!new_parent) | ||
| return TEE_ERROR_ITEM_NOT_FOUND; | ||
|
|
||
| old_parent = clk_get_parent(clk); |
There was a problem hiding this comment.
can you mark it as curr_parent for better readability
There was a problem hiding this comment.
will update this in upcoming iterations Vinod Kumar Amanaganti (@vinod-amanaganti)
Lemans has no secure DT, so register each QUPv3 SE clock as a plain struct clk, modeling the PLL/RCG/branch as separate objects so the generic framework's own refcounting governs each PLL vote. Signed-off-by: Naresh Nunna <nnunna@qti.qualcomm.com> Assisted-by: Claude Opus 5 Assisted-by: Claude Sonnet 5
b925381 to
91028d4
Compare
| clk->parent = new_parent; | ||
| } | ||
|
|
||
| static TEE_Result rcg_set_rate(struct clk *clk, unsigned long rate, |
There was a problem hiding this comment.
Could you please investigate whether this is a valid concern: unlike rcg_enable() and rcg_disable() (which both early-return on priv->dfs), rcg_set_rate() has no priv->dfs guard. If a consumer calls clk_set_rate() after qcom_clk_enable_dfs() has enabled hardware DFS on this RCG, config_mux_regs() + rcg_commit_config() would write CFG/CFG_UPDATE while the hardware performance-state machine owns the RCG (DFS_EN set), racing it. If that call ordering is possible for this clock, a priv->dfs guard here (as in enable/disable) may be warranted.
| continue; | ||
|
|
||
| rates[filled] = c->freq_hz; | ||
| dfs_indices[filled] = c->dfs_idx; |
There was a problem hiding this comment.
Could you please investigate whether this is a valid concern: qcom_clk_get_dfs_rates_array() validates !count and handles !rates (the size-query path), but never checks dfs_indices before writing dfs_indices[filled] here. A caller that passes a non-NULL rates buffer but a NULL dfs_indices would hit a NULL dereference. If dfs_indices is meant to be optional, a guard is needed; if it's mandatory, rejecting NULL up front (alongside the other param checks) would make the contract explicit.
|
Address above two comments, rest looks good. |
This series adds QUPv3 serial-engine (QUP SE) clock support to the Qualcomm
lemans target, for on-demand use by a TEE-side SPI/I2C driver.